Add convenience constructors to JSONRPCRequest #336
Conversation
|
need to rebase and run apiDump again |
kpavlov
force-pushed
the
kpavlov/JSONRPCRequest-constructors
branch
from
36a0d0e to
8909dee
Compare
kpavlov
force-pushed
the
kpavlov/JSONRPCRequest-constructors
branch
from
878d595 to
9e78bda
Compare
tiginamaria
left a comment
tiginamaria
left a comment
There was a problem hiding this comment.
Looks nicer now! Thank you!
|
set to draft to avoid conflicts with @devcrocod's changes |
…t-constructors # Conflicts: # kotlin-sdk-client/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StreamableHttpClientTransportTest.kt # kotlin-sdk-core/api/kotlin-sdk-core.api # kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt
- Updated `JSONRPCRequest` with new string and numeric ID constructors. - Replaced atomic ID handling with UUID defaults. - Enhanced test coverage for JSONRPCRequest creation scenarios.
devcrocod
left a comment
devcrocod
left a comment
There was a problem hiding this comment.
The request id is used to store handlers, and it’s also used as a progress token. Are you sure this change won’t break tracking of previous requests or progress notifications?
|
I am worrying that it is much harder to guarantee uniqueness of numeric field. UUID is a proven solution for such cases and migrating to UUID should be safe. Even more, we should switch from String to UUID internally to guarantee ID uniqueness. |
No, we can receive and process ids from various servers and clients (not only ours) |
Motivation and Context
To create request objects more easily
How Has This Been Tested?
Unit tests
Breaking Changes
No, this is a new API
Types of changes
Checklist
Additional context